Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add icons for sports pitches #3578

Closed
wants to merge 16 commits into from
Closed

[WIP] Add icons for sports pitches #3578

wants to merge 16 commits into from

Conversation

Adamant36
Copy link
Contributor

@Adamant36 Adamant36 commented Dec 19, 2018

This PR adds icons for sports pitches. Its related to #844 but does not resolve it. Since it only involves pitches. Other sports icons besides ones attached to pitches involve other things that are more complicated. So they will be dealt with in another PR. Along with sports centers.

One caveat about this PR is that names don't seem to render properly. As it is, they sometimes render at above the rendering level of the icons or sometimes not at all, but never with the icons.I don't know why there is the inconsistency. It might be something on my end with the code or it could be related to other issues with icons getting in the way of rendering names. As such, id appreciate it if someone could look through the code or test it to figure out what the problem might be. As I don't know enough about the subject myself. Although, I will look into it further and fix it if I can anyway. I figured it was still worth doing the PR in the meantime though.

american_football (all test renderings are at z18)
https://www.openstreetmap.org/#map=18/38.23502/-122.11689 (way)
american football z18
archery
https://www.openstreetmap.org/#map=18/37.05016/-122.06383 (way)
archery z18 way
https://www.openstreetmap.org/#map=18/36.99722/-121.99118 (node)
archery z18 node
badminton
https://www.openstreetmap.org/#map=19/37.37623/-122.19847 (way)
badminton z18 way
https://www.openstreetmap.org/#map=18/36.99125/-122.06386 (node)
badminton z18 node
baseball
https://www.openstreetmap.org/#map=18/38.23317/-122.11794 (way)
baseball z18
basketball
https://www.openstreetmap.org/#map=19/37.80742/-122.28172 (way)
basketball z18
handball
https://www.openstreetmap.org/#map=18/37.42064/-122.11221 (way)
handball z18
running
https://www.openstreetmap.org/#map=18/37.59517/-122.39120 (way)
running z18
Shooting
https://www.openstreetmap.org/#map=18/37.58821/-122.32339 (way)
shooting z18
skateboarding
https://www.openstreetmap.org/#map=18/37.59583/-122.38964 (way)
skateboard z18
soccer
https://www.openstreetmap.org/#map=18/38.23360/-122.12001 (way)
soccer z18
table tennis
https://www.openstreetmap.org/#map=18/37.71591/-122.45565 (way)
table tennis z18
tennis
https://www.openstreetmap.org/#map=18/38.23590/-122.12113 (way)
tennis z18
volleyball
https://www.openstreetmap.org/#map=18/37.59496/-122.39029 (way)
volleyball z18

Also, some of them don't have test renderings for nodes because they are only suppose to be mapped as ways due to being tagged on pitches. So I don't do test for them and I'm not going to. Or places that were just tagged as the sport without a pitch tag added to it. Which don't display alone and probably shouldn't.

@Adamant36
Copy link
Contributor Author

@jeisenbe, can you test this and tell me what you think the problem with name rendering might be?

Copy link
Collaborator

@jeisenbe jeisenbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To display name labels along with the icon, it is necessary to set text-dy. This depends on the height of the icon, but 10 will work. Shorter icons may need less.

@Adamant36
Copy link
Contributor Author

Well, I was thinking that since pitches already have text rendering with text-dy and sport is piggy backing off of it, that sport would inherit its name rendering rules. I guess I'll have to create a new line of code for every sport in the text rendering section instead though. Thanks for looking at it.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Dec 19, 2018

You can combine all the sports under one feature.

Instead of this:

 [feature = 'leisure_pitch'][sport = 'american_football'][zoom >= 17] {
    marker-file: url('symbols/sport/american_football.svg');
    marker-fill: @leisure-green;
    marker-placement: interior;
    marker-clip: false;
  }

  [feature = 'leisure_pitch'][sport = 'archery'][zoom >= 17] {
    marker-file: url('symbols/sport/archery.svg');
    marker-fill: @leisure-green;
    marker-placement: interior;
    marker-clip: false;
  } 
etc...

Try this

  [feature = 'leisure_pitch'][zoom >= 17] {
    marker-fill: @leisure-green;
    marker-placement: interior;
    marker-clip: false;
    [sport = 'american_football'] { marker-file: url('symbols/sport/generic-sport.svg'); }  \\ add generic sport icon
    [sport = 'archery'] { marker-file: url('symbols/sport/archery.svg'); }
... etc.
    [sport = 'soccer'] { marker-file: url('symbols/sport/soccer.svg'); }
... etc.
  }

You can use the same idea for the text, if you don't use a generic sport icon:

      [feature = 'leisure_pitch'] {
        text-fill: darken(@pitch, 40%);
        [sport = 'american_football'],
        [sport = 'archery'],
... etc.
        [sport = 'soccer']  {
          text-dy: 10;
        }
      }

@Tomasz-W Tomasz-W mentioned this pull request Dec 19, 2018
26 tasks
@Tomasz-W
Copy link

Tomasz-W commented Dec 19, 2018

I'm thinking how would it match with planned green dots for sports centres (currently they have different rendering of nodes and different of areas). At the other hand, if we will agree about green icons for some sport centres, the same green icons for pitches would be a bad idea then. @Adamant36 What is your vision here?

@Adamant36
Copy link
Contributor Author

Adamant36 commented Dec 19, 2018

@kocio-pl, would it be possible to move sports related things into their own sports.mss file? It would make it a lot easier to organize and implement this stuff. Especially with the other icons that are going to be added after this, adding icons to sports centers, issues for toboggans, golf stuff, pitch outlines etc. Otherwise its going to get pretty convoluted. The German style has one and the amenity points file is already pretty long in the tooth as it is.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Dec 19, 2018

@Tomasz-W, good question. I'll have to think about it. Both pitches and sports centers are leisure tags. So is there a reason they can't both be green?

I guess unifying sports center rendering would be good.

The problem with using a different color besides leisure green for the icons on pitches is that their names currently render in green. So the color on pitches without the sport tag would have to be modified. Otherwise, it wouldn't be consistent. There's also other sports tags that don't use pitches and are tagged on buildings like billiards. If we used a vastly different color for those it might be confusing. Plus, there's other things like name rendering on tracks mapped as lines. Since green is already being used on all those things and its established I don't see what the issue with using it on icons also would be. Otherwise, its going to have to be changed for everything else to. It could at least be lightened up a little if nothing else.

We could always forgo the fact that its all connected to a leisure tag though and create a different "sports" color category for names and icons related to sports. Like pure white or light blue or something. Id be fine with that to. As long as its consistent. Maybe it could be a different color for sports centers versus pitches though, but I'm not really sure what the difference is.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 19, 2018

would it be possible to move sports related things into their own sports.mss file?

I guess it is preferable, since it is only concatenated just before compiling and should not add any speed penalty, while making things more clear (which is quite different from data layers).

We could also probably try to separate shops into shops.mss to make amenity-points.mss easier to manage.

And no matter what color we would use for sport, it makes sense to create color variable with that name. In the simplest case this will be just the same color as leisure.

@Adamant36
Copy link
Contributor Author

Sweet. Shops was the other one I've been seriously considering doing it with.

I'll probably move everything into its own file before this is finalized. It seems like it will take a while to get the color thing figured out anyway.

@kocio-pl
Copy link
Collaborator

Just make sure to create separate PRs, since any relocations are error prone and much harder to check than adding or removing code.

@jeisenbe
Copy link
Collaborator

The green icons match well with the green color of pitches, and with the green name labels.

But I agree it looks a little unusual when a building is covering the pitch, and the sport icon now shows on the building, while the pitch area is not visible, eg:

Perhaps @Adamant36 could add [is_building = 'no']? This could prevent some of the cases where the sport icon is rendering in a building. eg

[feature = 'leisure_pitch'][is_building = 'no'][zoom >= 17] { marker-fill: @leisure-green; ... etc.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Dec 20, 2018

That might be a good idea. I was kind of wondering if rendering icons for stuff inside buildings like that is even necessary. I wouldn't consider the inside of a building a pitch and it seems like a little excessive. There's no reason instances like that can't be tagged with sports center or something instead and have the sport tag added to it that way.

I think is_building goes in the project file though doesn't it? or can it go anywhere?

@kocio-pl
Copy link
Collaborator

For detecting pitches inside the buildings indoor=* namespace would be useful, but that might need some discussion and documentation changes.

@Tomasz-W
Copy link

Tomasz-W commented Dec 21, 2018

I propose to go with leisure-green icons, and then take care of sport centres. If further test renderings will show that both leisure-green pitches icons and sport centres icons would cause mess, we would change pitches icons to man-made-grey.

Btw. I'm not a coder so I might be wrong, but I'm worried that there could be a problem of #3284 with icons for sports centres areas. If I understand it correctly, we can add icons only for sports centres nodes, am I right?

@jeisenbe
Copy link
Collaborator

jeisenbe commented Dec 21, 2018 via email

@Adamant36
Copy link
Contributor Author

Not that I disagree per say, but there are things out there with only node rendering that are also mapped as areas.

@kocio-pl
Copy link
Collaborator

I think the current code is mergable, both nodes and areas with green icons are OK for me. If you want to design some more changes to the rendering of pitches and sport centers, please open new tickets for that.

@Adamant36
Copy link
Contributor Author

Sweet. I was actually going to ask if you could merge it as is and we can workout the other stuff in a new issue later. There's much going on right now to get hung up on one thing.

@kocio-pl
Copy link
Collaborator

I think they should be rendered from z18. z17 is quite general high zoom level, it's not suitable for such details.

@Adamant36
Copy link
Contributor Author

@kocio-pl, your probably right. I updated it.

@kocio-pl
Copy link
Collaborator

Thanks.

It looks like there is a problem with labels - I started with american football and found that when we have the icon now, the label does not show at all. This is true also in your own example:

https://www.openstreetmap.org/way/174272803

@jeisenbe
Copy link
Collaborator

jeisenbe commented Dec 27, 2018 via email

@Adamant36
Copy link
Contributor Author

Oh yeah, thanks for bringing that up again. I totally forgot about it.

@Adamant36
Copy link
Contributor Author

The thing I don't get is that pitches already have their name displayed. So I don't know why the icons wouldn't piggyback off of that. Especially since the names still rendered at levels above the icon displaying and its further down in the code.

When I tried adding the sport features to the text part it didn't do anything. So can you do me a favor and test your solution to see if it actually works? Then copy the code here so I can paste it into the file since I couldn't get it to work myself? Otherwise, maybe it would be worth merging as is and then someone can start a new issue about it. Or we can wait forever for me to figure it out on my own.

@kocio-pl
Copy link
Collaborator

When it comes to icons:

  • I propose to flip runner icon so it's more distinct from fitness station
  • basketball icon is too strong and it could be made lighter by making white rectangle inside wider (probably 2 px instead of 1 px)

@Adamant36
Copy link
Contributor Author

Ok. Tomasz-W, you want to do that?

@jragusa
Copy link
Contributor

jragusa commented Dec 27, 2018

The handle of the paddle for the tennis table is also too long in proportion.

@Tomasz-W Tomasz-W mentioned this pull request Jan 1, 2019
19 tasks
@Tomasz-W
Copy link

Tomasz-W commented Jan 1, 2019

Sorry guys, as I have a lot of more important things on my head now (university stuff, organising charity action, family meetings...), I can't promise when I'll provide fixed icons and icons for more sports. Meanwhile, please discuss about wanted sport=* tags with dedicated icon and make a clear tasklist for me, it would be easier to understand what I have to do.

@Hufkratzer
Copy link

Since @Tomasz-W has no time and @Adamant36 wants to merge this soon, I make proposals for the 2 missing icons that @kocio-pl asked for above:

Probably not perfect but can perhaps be improved later when more icons are added.

@Adamant36
Copy link
Contributor Author

@Hufkratzer, thanks for the updated icons. I'll test them as soon as I get the time.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Jan 2, 2019

New running and basketball icons in the same place as the original running icon test rendering here. They fine to me. If they are good I'll update the commit so it can be further tested and merged.
download

@Tomasz-W
Copy link

Tomasz-W commented Jan 2, 2019

@Hufkratzer Thanks for working on these icons, but you have to remember about our standards:

  • if you want to cut some part of icon, draw a solid-white shape on it, then select both black and white parts and then click Path -> Difference (you used shape transparency option there, but this white shapein the middle should be just cutted out of bigger black shape)
  • after cutting select all elements and click Path -> Union (they should be one part)
  • save file in "Clean SVG" format (2nd one on the "Save as" list)
  • if code of your icon would be still much longer than e.g. code here, you can use this icon file as a template file and copy designed icon there, it should be shorter then

@Adamant36
Copy link
Contributor Author

Thanks @Tomasz-W. Guess I'll retest them after they get fixed. Sigh.

@Hufkratzer
Copy link

@Tomasz-W
Sorry, I don't understand, please explain again what is wrong:

  • My running.svg is just a flipped version of your running.svg, I opend yours, flipped it and saved it as a normal svg; it is not longer than yours. What else is missing?
  • My basketball.svg is a single path (built with difference and union) and then I have pasted this into the background of some other icon as you recommended here, is not longer than this basketball.svg. I am not sure if the white canvas is needed when the icon itself is 14x14 already, I can take it out, but some other icons (e.g. toys.svg, playgroind.svg, beach_resort.svg) have it too. I don't undestand what else could be wrong.

@Adamant36
Please check if you have tested the latest version of my basketball icon, I edited it 2 times yesterday.

@Adamant36
Copy link
Contributor Author

I tested whatever icon was in the link four hours ago. There was only one.

@Hufkratzer
Copy link

Then you tested the currently latest revision.

But does it make sense to render pitch + running with an icon at all? Isn't that mistagging and running only to be used with track? Compare https://wiki.openstreetmap.org/wiki/Tag:sport=running.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Jan 2, 2019

Hhmmm, I dont know. Its a good question. The says a pitch is "An area designed for playing a particular sport, normally designated with appropriate markings." it seems like running tracks fit that and its not mentioned as misstagging on the track page. There's a thousandish instances of sport=running. So...Personally, id think they could co exist. Since a track is a type of pitch, but maybe it needs disscussion somewhere (tagging mailing list?).

Im impartial since I dont particularly like having an icon for tracks anyway. Since its pretty obvious thats what they are and it seems like the position of the icon is random.

I would like to see this done at some point though. My current VM and Docker setup is semi screwed up (plus, I dont like Ubuntu version im using anyway) and I need to reinstall everything to get it back to normal again..

@dieterdreist
Copy link

dieterdreist commented Jan 3, 2019 via email

@Adamant36
Copy link
Contributor Author

@dieterdreist, I'm fine removing it then. I guess the only other thing is figuring out the problem with the basketball icon if there is one.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jan 3, 2019

I also think that running tracks/pitches might be best left without any icon, it's pretty clear from the shape (horse running should have different context and maybe icon too) and they are smaller, so it would be a bit too much.

@kocio-pl kocio-pl changed the title Add icons for sports pitches [WIP] Add icons for sports pitches Jan 3, 2019
@jragusa
Copy link
Contributor

jragusa commented Jan 3, 2019

Problems happen especially with running polygons in stadiums. Icon should be located on the middle and then overlaps the pitch in the centre of the stadium.

@Adamant36
Copy link
Contributor Author

@Tomasz-W, when you get some time would you mind fixing the basketball icon since your the one that seems to know all this stuff?

@Tomasz-W
Copy link

Tomasz-W commented Jan 4, 2019

Sure, I think I'll do it within next week.

@Adamant36
Copy link
Contributor Author

@Tomasz-W, any update on the icon?

@matthijsmelissen, are you going to reject this for being to detailed/extra icons/whatever when it comes time? (I don't mind if you do, I just thought id ask since I noticed your issue about icons and it might qualify as over detailed, maybe?).

@Tomasz-W
Copy link

@Adamant36 Updated and reviewed icons:

@matthijsmelissen
Copy link
Collaborator

@Adamant36 I think I'm going to postpone making a decision on this PR (either way) until after the discussion has finished (hopefully not too long).

@Adamant36
Copy link
Contributor Author

Personally, I think for the sake of fairness to the people that work into this over the last few years (not just me, and the fact that it had four years worth of being open for a maintainer to close it if they wanted, it shouldnt be considered in connection to the current disscussion about not adding new features and should judged on its own merrets (Surely map clutter cant be a valid reason to add it as there's no other icons in the middle of sports pitches that they would colude with).

@boothym
Copy link
Contributor

boothym commented Jan 17, 2019

Just wanted to say thanks for the work in this PR, the icons look good and I think they will be a welcome addition to the map.

@Adamant36
Copy link
Contributor Author

Your welcome. Im just waiting for them to get the meta issue dealing with adding new icons or not figured out. Then I'll update it so it can be reviewed and merge. I might update it soon either way though.

@Adamant36 Adamant36 closed this Jan 18, 2019
@Adamant36 Adamant36 deleted the sport branch January 18, 2019 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.